Tab5 power expander driver and devicetree parsing improvements#507
Tab5 power expander driver and devicetree parsing improvements#507KenVanHoeylandt merged 10 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CLI script to extract devicetree dependencies and updates the devicetree code generator to emit forward declarations and a null-terminated 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TactilityKernel/include/tactility/kernel_init.h (1)
11-19:⚠️ Potential issue | 🟡 MinorStale
@param platform_modulein doc comment — parameter was removed.The function signature no longer has a
platform_moduleparameter, but Line 13 still documents it. Update the doc block to match the actual signature.📝 Proposed fix
/** - * Initialize the kernel with platform and device modules, and a device tree. - * `@param` platform_module The platform module to start. This module should not be constructed yet. + * Initialize the kernel with device modules and a device tree. * `@param` device_module The device module to start. This module should not be constructed yet. This parameter can be NULL. * `@param` dts_modules List of modules from devicetree, null-terminated, non-null parameter * `@param` dts_devices The list of generated devices from the devicetree. The array must be terminated with DTS_DEVICE_TERMINATOR. This parameter can be NULL. * `@return` ERROR_NONE on success, otherwise an error code */
🧹 Nitpick comments (4)
Firmware/CMakeLists.txt (1)
24-27:pip installruns unconditionally on every CMake configure.This slows down every configure invocation. Consider gating it behind a stamp file or a cache variable so it only runs once (or when dependencies change).
Platforms/platform-esp32/source/drivers/esp32_i2c.cpp (1)
248-257: Minor inconsistency: missingstructkeyword in extern declaration.Other ESP32 driver files (esp32_i2s.cpp, esp32_gpio.cpp, esp32_uart.cpp) use
extern struct Module platform_esp32_module;, but this file omits thestructkeyword. While valid C++, it's inconsistent.Suggested fix
-extern Module platform_esp32_module; +extern struct Module platform_esp32_module;Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp (2)
14-26: Inconsistent indentation: mixed tabs and spaces.Lines 15 and 21 use tabs while lines 16–20 use spaces. The rest of the codebase uses spaces consistently.
Suggested fix
static error_t start(Device* device) { - auto* parent = device_get_parent(device); + auto* parent = device_get_parent(device); if (device_get_type(parent) != &I2C_CONTROLLER_TYPE) { LOG_E(TAG, "Parent device is not I2C"); return ERROR_RESOURCE; } LOG_I(TAG, "Started PI4IOE5V6408 device %s", device->name); - return ERROR_NONE; + return ERROR_NONE; } static error_t stop(Device* device) { - return ERROR_NONE; + return ERROR_NONE; }
14-41:start/stopdefined outsideextern "C"— inconsistent with other drivers.All other driver files in this codebase (e.g.,
esp32_i2s.cpp,esp32_gpio.cpp,esp32_uart.cpp) define their static functions inside theextern "C"block. While this works for static functions (internal linkage), it's inconsistent with the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Firmware/CMakeLists.txt (2)
88-89: Hardcoded driver source file breaks the DTS abstraction.
symbols.cfrompi4ioe5v6408-moduleis added directly as a source file, bypassing the DTS dependency mechanism used in the ESP path (lines 69–72). This won't scale as more DTS-driven modules are added and creates a maintenance burden for the simulator build.Consider either wiring the non-ESP path through DTS dependencies the same way, or at minimum adding a comment explaining why this workaround is necessary for the simulator build.
24-27:pip installruns on every CMake configure.This slows down reconfiguration. Consider guarding this with a cache variable or a stamp file so it only runs once (or when versions change).
Drivers/pi4ioe5v6408-module/include/drivers/pi4ioe5v6408.h (1)
12-15: Consider renamingregtoaddressfor clarity.The field represents an I2C bus address (as your comment notes), but
regconventionally implies a register offset in I2C contexts. This could confuse maintainers who seeGET_CONFIG(device)->regpassed as the address argument toi2c_controller_register8_*helpers.Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp (2)
13-21: Unused register constants.
PI4_REGISTER_PULL_ENABLE,PI4_REGISTER_PULL_SELECT,PI4_REGISTER_INTERRUPT_MASK, andPI4_REGISTER_INTERRUPT_LEVELare defined but never used. Fine to keep if you plan to use them soon; otherwise they're dead code.
39-62: Inconsistent use ofstruct Device*vsDevice*across the API functions.Lines 39, 49, 54 use
struct Device*while lines 44, 59 use bareDevice*. Both work in C++extern "C"blocks, but the inconsistency is noticeable. Consider picking one form for uniformity — ideally matching the header declaration style (struct Device*).
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (44)
Devices/elecrow-crowpanel-advance-50/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorStale
@warningcomment contradicts the new symbol name.The comment on Line 15 asserts that the variable name must be exactly
"device_module", but the variable was just renamed toelecrow_crowpanel_advance_50_module. The comment should be updated or removed to avoid misleading future maintainers.📝 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ -struct Module elecrow_crowpanel_advance_50_module = { +struct Module elecrow_crowpanel_advance_50_module = {Devices/waveshare-esp32-s3-geek/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the stale
@warningcomment.The comment on Line 15 says the variable name must be exactly
"device_module", but the variable has just been renamed towaveshare_esp32_s3_geek_module. This comment is now incorrect and actively misleading.✏️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module waveshare_esp32_s3_geek_module = {Devices/lilygo-tdeck/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorStale
@warningcomment contradicts the rename.The doc comment on Line 15 still says the variable name must be exactly
"device_module", but the variable was just renamed tolilygo_tdeck_module. This warning should be updated or removed to avoid misleading future contributors.✏️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module lilygo_tdeck_module = {Devices/generic-esp32p4/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-contradictory
@warningcomment.The warning on Line 15 states the variable must be named exactly
"device_module", but the variable has just been renamed togeneric_esp32p4_module. The comment is stale and actively misleading — it should be removed (or updated if a new naming contract exists for this symbol).🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module generic_esp32p4_module = {Devices/heltec-wifi-lora-32-v3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the stale
@warningcomment.The comment on Line 15 asserts the variable name must be exactly
"device_module", but the variable was just renamed toheltec_wifi_lora_32_v3_module. This contradicts the change and will mislead future readers.🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module heltec_wifi_lora_32_v3_module = {Devices/waveshare-s3-touch-lcd-128/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the stale
@warningcomment — it now contradicts the renamed symbol.Line 15 still asserts
"The variable name must be exactly "device_module"", but the variable was just renamed towaveshare_s3_touch_lcd_128_moduleon Line 16. This is actively misleading for anyone touching this file in the future.🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module waveshare_s3_touch_lcd_128_module = {Devices/waveshare-s3-lcd-13/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-contradictory
@warningcomment.The comment on Line 15 asserts the variable name must be
"device_module", but the variable was just renamed towaveshare_s3_lcd_13_module. Leaving it in place will actively mislead future maintainers.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module waveshare_s3_lcd_13_module = {Devices/cyd-e32r28t/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-contradictory
@warningcomment.The comment on Line 15 states the variable name must be exactly
"device_module", but the variable has been intentionally renamed tocyd_e32r28t_moduleas part of the PR-wide convention change. The comment is now stale and misleading.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module cyd_e32r28t_module = {Devices/guition-jc1060p470ciwy/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the now-contradictory
@warningcomment.The comment on Line 15 explicitly states the variable name must be
device_module, but the variable was just renamed toguition_jc1060p470ciwy_module. This is actively misleading and should be removed or updated to reflect the new naming convention.📝 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module guition_jc1060p470ciwy_module = {Devices/wireless-tag-wt32-sc01-plus/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the stale
@warningcomment.The comment on line 15 still says the variable name must be
"device_module", but the symbol was intentionally renamed towireless_tag_wt32_sc01_plus_module. Leaving it in place directly contradicts the current code and will confuse future readers.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module wireless_tag_wt32_sc01_plus_module = {Devices/cyd-2432s032c/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove/update the stale
@warningcomment.The warning on Line 15 says the variable name must be exactly
"device_module", but the variable has just been renamed tocyd_2432s032c_module. The comment now contradicts the code and will mislead future readers.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module cyd_2432s032c_module = {Devices/m5stack-cores3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the now-stale
@warningcomment.The comment on line 15 explicitly mandates that the variable be named
"device_module", but the variable was intentionally renamed tom5stack_cores3_moduleby this PR. Leaving it in will mislead future contributors into thinking the old name is still a hard requirement.🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_cores3_module = {Devices/lilygo-tdisplay-s3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-stale
@warningcomment — it contradicts the rename.The comment on line 15 asserts the variable name must be
"device_module", but the variable has been intentionally renamed tolilygo_tdisplay_s3_module. Leaving this warning in place will mislead future contributors into thinking the rename is wrong or must be reverted.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module lilygo_tdisplay_s3_module = {Devices/guition-jc3248w535c/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the now-stale
@warningcomment.The comment on Line 15 still asserts the variable name must be exactly
"device_module", directly contradicting the rename on Line 16. This will mislead future readers and any tooling that keys off it.📝 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module guition_jc3248w535c_module = {Devices/elecrow-crowpanel-basic-50/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the stale
@warningcomment.The comment on line 15 states the variable name must be
"device_module", but the variable has been intentionally renamed toelecrow_crowpanel_basic_50_moduleas part of the PR-wide migration to per-module named symbols. The comment now directly contradicts the code and would mislead any developer who encounters it.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module elecrow_crowpanel_basic_50_module = {Devices/cyd-4848s040c/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-contradictory
@warningcomment.The comment on line 15 asserts that the variable name must be
"device_module", but the PR intentionally renames it tocyd_4848s040c_module. Leaving the comment in place actively contradicts the code and will mislead future readers.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module cyd_4848s040c_module = {Devices/lilygo-tdisplay/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-contradictory
@warningcomment.The comment on line 15 asserts the symbol must be
"device_module", but line 16 renames it tolilygo_tdisplay_module. The broader PR intent (switching to a nameddts_modules[]array) is precisely what removes that fixed-name requirement, so the warning is now both stale and actively misleading. It should be deleted.🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module lilygo_tdisplay_module = {Devices/m5stack-core2/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the now-contradictory
@warningcomment.The comment on line 15 says the variable name must be
"device_module", but the variable was just renamed tom5stack_core2_module. This is directly misleading to future maintainers. Since the PR's intent is precisely to move away from the genericdevice_modulename, the comment should be dropped (or reworded if there's still a linkage constraint it's trying to document).🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_core2_module = {Devices/generic-esp32c6/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorStale comment contradicts the rename.
Line 15 still says the variable name must be exactly
"device_module", but it was renamed togeneric_esp32c6_module. Update or remove this@warningto match the new naming convention.Suggested fix
-/** `@warning` The variable name must be exactly "device_module" */ -struct Module generic_esp32c6_module = { +struct Module generic_esp32c6_module = {Or, if the constraint still applies under the new scheme:
-/** `@warning` The variable name must be exactly "device_module" */ +/** `@warning` The variable name must match the symbol expected by the devicetree code generator */ struct Module generic_esp32c6_module = {Devices/guition-jc8048w550c/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove or update the now-contradictory
@warningcomment.Line 15 still states
/**@warningThe variable name must be exactly "device_module" */, but the variable on line 16 has been renamed toguition_jc8048w550c_module. The comment is now stale and misleading — it implies the old generic name is required when the entire PR is moving away from it.🧹 Proposed fix — drop the stale comment
-/** `@warning` The variable name must be exactly "device_module" */ struct Module guition_jc8048w550c_module = {Devices/cyd-2432s024c/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorStale
@warningcomment contradicts the rename.Line 15 still reads
The variable name must be exactly "device_module", but the symbol is nowcyd_2432s024c_module. This will mislead future contributors into thinking the name is constrained. Please update or remove the comment to reflect the new naming convention driven by the devicetree generator.Suggested fix
-/** `@warning` The variable name must be exactly "device_module" */ +/** `@warning` The variable name must match the symbol expected by the devicetree module list */ struct Module cyd_2432s024c_module = {Devices/m5stack-cardputer-adv/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorStale
@warningcomment contradicts the renamed symbol.The comment on Line 15 still says the variable name must be exactly
"device_module", but the variable was just renamed tom5stack_cardputer_adv_module. This comment is now actively misleading and should be updated or removed.📝 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_cardputer_adv_module = {Devices/m5stack-cardputer/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorSame stale
@warningcomment as the-advvariant.The comment on Line 15 still references
"device_module"while the actual symbol is nowm5stack_cardputer_module. Update or remove it.📝 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_cardputer_module = {Devices/elecrow-crowpanel-advance-28/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-contradictory
@warningcomment.The comment on line 15 asserts the variable name must be
"device_module", but the variable was just renamed toelecrow_crowpanel_advance_28_module. Leaving this comment in place is actively misleading for anyone who reads or modifies this file later.🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module elecrow_crowpanel_advance_28_module = {Devices/generic-esp32/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the now-stale
@warningcomment — it contradicts the rename.The comment on Line 15 asserts the variable name must be
"device_module", but the variable was just renamed togeneric_esp32_module. Since the PR migrates the kernel init flow from a singledevice_module/platform_moduleconvention to adts_modules[]array lookup, this naming constraint no longer applies and the comment is actively misleading.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module generic_esp32_module = {Devices/m5stack-stickc-plus/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorSame stale
@warningcomment — remove or update.Identical to the plus2 file: the comment still reads
"must be exactly device_module"while the symbol is nowm5stack_stickc_plus_module.🗑️ Proposed fix: remove the stale warning
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_stickc_plus_module = {Devices/m5stack-stickc-plus2/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorStale
@warningcomment contradicts the renamed symbol.The doc comment on line 15 still says the variable name must be exactly
"device_module", but the variable has been renamed tom5stack_stickc_plus2_module. Either remove the comment entirely (since the new naming convention makes it obsolete) or update it to reflect the new requirement.🗑️ Proposed fix: remove the stale warning
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_stickc_plus2_module = {Devices/unphone/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale
@warningcomment — variable naming is no longer constrained.The
@warningon line 15 asserts the name must be"device_module", but this constraint no longer applies. The devicetree compiler (Buildscripts/DevicetreeCompiler/source/generator.py) dynamically generatesdts_modules[]by collecting references to whatever symbol is actually defined in each device module—no fixed naming requirement. Theunphone_modulerename is safe and has no external callers.The warning contradicts the code and should be removed to prevent future confusion.
Devices/generic-esp32s3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the stale
@warningcomment that contradicts the current variable name.The variable has been renamed to
generic_esp32s3_module, but the@warningon line 15 still claims it must be exactly"device_module". This comment is now contradictory dead documentation and should be removed.Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module generic_esp32s3_module = {Devices/elecrow-crowpanel-basic-35/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the stale
@warningcomment — it directly contradicts the renamed variable.Line 15 asserts the symbol must be named
device_module, but the variable is nowelecrow_crowpanel_basic_35_module. This comment appears to be a template that was not updated when the device symbol was renamed. Verification confirms no references to the olddevice_modulename exist for this device, so the rename is clean. Remove the misleading comment.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module elecrow_crowpanel_basic_35_module = {Devices/elecrow-crowpanel-advance-35/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale warning comment that contradicts the device-specific variable name.
The
@warningcomment on line 15 states the variable must be exactly"device_module", but the variable is intentionally namedelecrow_crowpanel_advance_35_moduleto be device-specific. This comment is stale and misleading.Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module elecrow_crowpanel_advance_35_module = {Devices/lilygo-tdongle-s3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale
@warningcomment that contradicts the renamed symbol.The warning on line 15 mandates
"device_module"as the exact variable name, but the symbol is nowlilygo_tdongle_s3_module. No external code references the old name, so the warning is outdated and misleading.Suggested fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module lilygo_tdongle_s3_module = {Devices/m5stack-papers3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the stale
@warningcomment on line 15.The comment states the variable name must be
"device_module", but the variable has been renamed tom5stack_papers3_module. This is actively misleading and should be deleted. Verification confirms m5stack-papers3 has no orphaned extern references to the old symbol.Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module m5stack_papers3_module = {Devices/simulator/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale
@warningcomment — the variable has been renamed tosimulator_module.Line 15's warning states the variable name must be exactly
"device_module", but the actual variable on line 16 is namedsimulator_module. No extern declarations or bindings in the codebase reference adevice_modulefor the simulator device, confirming the rename is complete and the comment is outdated. Delete the warning comment.Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module simulator_module = {Devices/waveshare-s3-touch-lcd-147/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale warning comment that contradicts the renamed variable.
The
@warningcomment on Line 15 states the variable name "must be exactlydevice_module", but the variable is nowwaveshare_s3_touch_lcd_147_module. This comment is no longer accurate—no code in this device depends on the old name. Remove it.Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module waveshare_s3_touch_lcd_147_module = {Devices/cyd-8048s043c/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale
@warningcomment — the constraint no longer applies to this device.The
@warningon line 15 is outdated. The variable has been renamed tocyd_8048s043c_moduleand there are no hardcoded dependencies on the exact name"device_module"within this device's code. The comment actively misleads maintainers into thinking the name is a hard requirement when it is not.-/** `@warning` The variable name must be exactly "device_module" */ struct Module cyd_8048s043c_module = {Devices/cyd-2432s028rv3/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove the stale
@warningcomment on Line 15.The comment asserts the variable name must be exactly
"device_module", but the actual variable is namedcyd_2432s028rv3_module. This contradictory comment will mislead anyone reading it. Since no external code references this module by a generic symbol name, the comment can be safely removed.🧹 Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module cyd_2432s028rv3_module = {Devices/waveshare-s3-touch-lcd-43/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorSame stale
@warning— remove it.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module waveshare_s3_touch_lcd_43_module = {Devices/lilygo-tlora-pager/Source/module.cpp-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorSame stale
@warningas the other renamed modules — remove it.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module lilygo_tlora_pager_module = {Devices/btt-panda-touch/Source/module.cpp-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove stale
@warning— the constraint it describes no longer applies.The comment says the variable must be exactly
"device_module", but the variable has just been renamed tobtt_panda_touch_module. The warning is now factually wrong and will mislead future contributors.🗑️ Proposed fix
-/** `@warning` The variable name must be exactly "device_module" */ struct Module btt_panda_touch_module = {Tactility/Include/Tactility/Tactility.h-27-29 (1)
27-29:⚠️ Potential issue | 🟡 Minor
dtsDevicesnullability not reflected in the doc comment.
kernel_init.cppaccepts a nulldts_devicespointer (if (dts_devices)), so callers oftt::runmay legally passnullptr. The current@param dtsDevicesdoc only says it must be terminated withDTS_DEVICE_TERMINATOR, omitting that it can be null.📝 Proposed fix
- * `@param` dtsDevices Array that is terminated with DTS_DEVICE_TERMINATOR + * `@param` dtsDevices Array terminated with DTS_DEVICE_TERMINATOR, or nullptr if there are no devicetree devicesDevices/m5stack-tab5/Source/Configuration.cpp-68-68 (1)
68-68:⚠️ Potential issue | 🟡 Minor
= nullptrdefault ininitSoundcreates a dangerous API — the body unconditionally dereferencesio_expander0.If
io_expander0is omitted (or null after the missing expander check is fixed separately),pi4ioe5v6408_get_output_levelwill immediately dereference a null pointer. Either remove the default or guard the body.🔧 Option A — remove the default (preferred, since all callers supply the argument)
-static error_t initSound(::Device* i2c_controller, ::Device* io_expander0 = nullptr) { +static error_t initSound(::Device* i2c_controller, ::Device* io_expander0) {Drivers/pi4ioe5v6408-module/source/symbols.c-4-15 (1)
4-15:⚠️ Potential issue | 🟡 Minor
pi4ioe5v6408_get_output_levelmissing from the symbol export table.This function is declared in the public header and actively used by
Devices/m5stack-tab5/Source/Configuration.cpp(initSound), but it is not exported viapi4ioe5v6408_module_symbols. Any module performing a runtime symbol lookup for it will fail.🔧 Proposed fix
DEFINE_MODULE_SYMBOL(pi4ioe5v6408_set_pull_select), DEFINE_MODULE_SYMBOL(pi4ioe5v6408_get_input_level), + DEFINE_MODULE_SYMBOL(pi4ioe5v6408_get_output_level), DEFINE_MODULE_SYMBOL(pi4ioe5v6408_set_interrupt_mask),Devices/m5stack-tab5/Source/Configuration.cpp-116-125 (1)
116-125:⚠️ Potential issue | 🟡 MinorLog messages report stale
ERROR_NONEinstead of the actual failure code.
erroris assigned once fromi2c_controller_write_register_array. If that call succeeds,error == ERROR_NONEfor the rest of the function. The return values ofpi4ioe5v6408_get_output_levelandpi4ioe5v6408_set_output_levelare checked inline but never captured, so both error messages will always print"ERROR_NONE"regardless of what actually failed.🐛 Proposed fix
- if (pi4ioe5v6408_get_output_level(io_expander0, &output_level, pdMS_TO_TICKS(100)) != ERROR_NONE) { - LOG_E(TAG, "Failed to read power level: %s", error_to_string(error)); + error_t expander_error; + if ((expander_error = pi4ioe5v6408_get_output_level(io_expander0, &output_level, pdMS_TO_TICKS(100))) != ERROR_NONE) { + LOG_E(TAG, "Failed to read power level: %s", error_to_string(expander_error)); return ERROR_RESOURCE; } - if (pi4ioe5v6408_set_output_level(io_expander0, output_level | 0b00000010, pdMS_TO_TICKS(100)) != ERROR_NONE) { - LOG_E(TAG, "Failed to enable amplifier: %s", error_to_string(error)); + if ((expander_error = pi4ioe5v6408_set_output_level(io_expander0, output_level | 0b00000010, pdMS_TO_TICKS(100))) != ERROR_NONE) { + LOG_E(TAG, "Failed to enable amplifier: %s", error_to_string(expander_error)); return ERROR_RESOURCE; }
🧹 Nitpick comments (3)
Devices/simulator/CMakeLists.txt (1)
15-22: Consider consolidating the twotarget_link_librariescalls.Lines 15–20 and line 22 are two separate invocations for the same target. They can be merged into one, and it avoids the question of whether
SDL2-static(line 19) and${SDL2_LIBRARIES}(line 22) overlap.♻️ Proposed consolidation
- target_link_libraries(simulator + target_link_libraries(simulator PRIVATE Tactility PRIVATE TactilityCore PRIVATE lvgl PRIVATE SDL2-static + PRIVATE ${SDL2_LIBRARIES} ) - - target_link_libraries(simulator PRIVATE ${SDL2_LIBRARIES})Drivers/pi4ioe5v6408-module/include/drivers/pi4ioe5v6408.h (1)
14-17:regfield name conflicts with its comment.The comment "Address on bus" correctly describes the field's purpose (I2C device address), but the name
regconventionally implies a device register offset, not a bus address. Consider renaming toaddressori2c_addressfor clarity, especially since this struct is part of the public API.Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp (1)
89-98: Non-standard C++ compound literal for.compatible.
(const char*[]) { ... }is a C99 compound literal — not part of the C++ standard. GCC and Clang accept it as an extension and give it static storage duration at file scope, so it works on ESP-IDF today. A portable alternative avoids the extension entirely:♻️ Portable alternative
+static const char* pi4ioe5v6408_compatible[] = { "diodes,pi4ioe5v6408", nullptr }; + Driver pi4ioe5v6408_driver = { .name = "pi4ioe5v6408", - .compatible = (const char*[]) { "diodes,pi4ioe5v6408", nullptr}, + .compatible = pi4ioe5v6408_compatible, ... };
Keep the binding "reg" for consistency with common dts naming
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Devices/waveshare-s3-touch-lcd-147/Source/module.cpp (1)
15-21:⚠️ Potential issue | 🔴 CriticalSymbol is orphaned and unreachable — declaration and usage are missing.
The new symbol
waveshare_s3_touch_lcd_147_moduleis defined but never declared in a header or referenced anywhere in the codebase. It has C linkage (insideextern "C"), yet lacks anexterndeclaration. Either this refactoring is incomplete and the symbol needs to be declared in a public header and registered with the module system, or it is dead code that should be removed. Verify the intended integration point and either add the necessary declaration/registration or remove the unused symbol.Devices/lilygo-tlora-pager/Source/module.cpp (1)
23-29:⚠️ Potential issue | 🔴 CriticalUpdate references to the renamed module symbol in TloraPager.cpp.
The rename from
device_moduletolilygo_tlora_pager_moduleis incomplete. The fileDevices/lilygo-tlora-pager/Source/drivers/TloraPager.cppstill declares and uses the old symbol name:
- Line 9:
extern struct Module device_module;- Line 26:
.owner = &device_module,Both must be updated to use
lilygo_tlora_pager_moduleto match the new symbol name in module.cpp. Without this change, the build will fail with an unresolved symbol error at link time. The new.symbolsand.internalfield additions are correct.Devices/elecrow-crowpanel-advance-28/Source/module.cpp (1)
15-20:⚠️ Potential issue | 🟡 MinorAdd
.internal = nullptrto the Module initializer.The
struct Modulehas an.internalfield that the header comment explicitly states should be initialized toNULLby module implementers. All sampled sibling module.cpp files in the repository (btt-panda-touch, cyd-2432s024c, generic-esp32, and others) include.internal = nullptrin their initializers. This file omits it, creating an inconsistency:Other modules for reference
// btt-panda-touch, cyd-2432s024c, generic-esp32, etc. struct Module xxx_module = { .name = "...", .start = start, .stop = stop, .symbols = nullptr, .internal = nullptr };While C++ zero-initializes unspecified designated initializer fields (so no runtime risk), follow the documented pattern.
Devices/cyd-4848s040c/Source/module.cpp (1)
15-21:⚠️ Potential issue | 🟠 MajorModule definition is correct but integration is incomplete.
The symbol rename to
cyd_4848s040c_moduleand expanded initializer are correctly implemented. However, the module is not properly integrated into the system:
- No public header declares
extern struct Module cyd_4848s040c_module(unlikehal_device_modulewhich hastactility/hal_device_module.h)- The symbol is not referenced from any application or initialization code
- The device tree file defines only hardware (GPIO/I2C/SPI), not the module itself
Following the pattern of
hal_device_module, the device module either needs to be declared in a public header and registered in the initialization code (e.g.,dts_modules[]array), or this incomplete integration must be clarified.
🧹 Nitpick comments (5)
Devices/guition-jc1060p470ciwy/Source/module.cpp (1)
19-20: Redundant explicitnullptrinitialisers.
.symbols = nullptrand.internal = nullptrare already zero-initialised by C++ aggregate initialisation when omitted. Keeping them is harmless, but they add noise; consider dropping them for consistency with other modules, or retaining them only if the team prefers explicit initialization as a documentation aid.♻️ Proposed simplification
struct Module guition_jc1060p470ciwy_module = { .name = "guition-jc1060p470ciwy", .start = start, .stop = stop, - .symbols = nullptr, - .internal = nullptr };TactilityKernel/source/kernel_init.cpp (1)
32-68: Bail-on-first-error approach — verify this is the desired behavior.
kernel_initreturnsERROR_RESOURCEon the first module or device that fails to initialize, leaving the remaining modules/devices uninitialized. If partial initialization is acceptable (or if some devices are optional), you may want a "best-effort" mode that logs failures and continues. If the current fail-fast approach is intentional (i.e., any failure is fatal), this is fine as-is.Devices/guition-jc8048w550c/Source/module.cpp (1)
15-21: LGTM — rename and new field initialisation look correct.The rename to
guition_jc8048w550c_moduleis consistent with board-specific naming, and the two new fields are properly zero-initialised.However, note: this module is not currently exported or referenced in any
dts_modules[]arrays. If this device is meant to be integrated into the module system, you'll need to add a public header withextern struct Module guition_jc8048w550c_module;(following the pattern used by other modules likehal_device_module) and include it in the appropriate device tree registration. If the module is only for internal device use, no further action is needed.Drivers/pi4ioe5v6408-module/source/pi4ioe5v6408.cpp (2)
39-51: Nit: inconsistentstruct Device*vsDevice*within theextern "C"block.The first three functions (lines 39, 44, 49) omit the
structkeyword while the remaining seven usestruct Device*. The public header usesstruct Device*uniformly throughout. Both spellings compile identically in C++, but unifying onstruct Device*here would match the header declarations and keep the style consistent within theextern "C"block.🔧 Proposed fix
-error_t pi4ioe5v6408_set_direction(Device* device, uint8_t bits, TickType_t timeout) { +error_t pi4ioe5v6408_set_direction(struct Device* device, uint8_t bits, TickType_t timeout) { -error_t pi4ioe5v6408_set_output_level(Device* device, uint8_t bits, TickType_t timeout) { +error_t pi4ioe5v6408_set_output_level(struct Device* device, uint8_t bits, TickType_t timeout) { -error_t pi4ioe5v6408_get_output_level(Device* device, uint8_t* bits, TickType_t timeout) { +error_t pi4ioe5v6408_get_output_level(struct Device* device, uint8_t* bits, TickType_t timeout) {
89-98: Optional: replace the compound literal with a named static array for standard C++.
(const char*[]) { ... }is a C99 compound literal — not valid in standard C++, though GCC and Clang accept it as an extension. The idiomatic, portable C++ alternative is a file-scoped static array:♻️ Proposed refactor
+static const char* const pi4ioe5v6408_compatible[] = { "diodes,pi4ioe5v6408", nullptr }; + Driver pi4ioe5v6408_driver = { .name = "pi4ioe5v6408", - .compatible = (const char*[]) { "diodes,pi4ioe5v6408", nullptr}, + .compatible = pi4ioe5v6408_compatible, .start_device = start, .stop_device = stop, .api = nullptr, .device_type = nullptr, .owner = &pi4ioe5v6408_module, .internal = nullptr };
1 participant
Summary by CodeRabbit
New Features
Device Tree Updates
Build / Tooling
Refactor
Documentation